fix(replay): Don't let a wedged video encoder freeze the app - #5842
fix(replay): Don't let a wedged video encoder freeze the app#5842romtsn wants to merge 2 commits into
Conversation
Some hardware encoders never emit BUFFER_FLAG_END_OF_STREAM after signalEndOfInputStream(), so SimpleVideoEncoder.drainCodec() spun forever while holding encoderLock. ReplayCache.close() then blocked on that lock, and since it runs inline under ReplayIntegration's lifecycleLock, the main thread froze until the system killed the process. Two bounds, both needed: the drain loop now gives up after 10 consecutive no-progress iterations (~1s), and close() only waits 2s for the encoder lock before skipping the release. The loop bound alone isn't enough -- a native dequeueOutputBuffer call can itself never return, since ALooper::awaitResponse has no deadline of its own. Adds AutoClosableReentrantLock.tryAcquire(timeout, unit) for the latter. Fixes getsentry/sentry-dart#3556 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| * [MediaCodec.signalEndOfInputStream], which used to spin the drain loop forever while holding the | ||
| * encoder lock, wedging the whole replay pipeline (and with it the app's lifecycle callbacks). | ||
| */ | ||
| private const val MAX_EOS_STALL_ITERATIONS = 10 |
There was a problem hiding this comment.
how did we determine 10 here? Just curious. How fast do these iterations happen?
📲 Install BuildsAndroid
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 326.06 ms | 385.46 ms | 59.40 ms |
| bb0ff41 | 315.84 ms | 350.76 ms | 34.92 ms |
| 806307f | 357.85 ms | 424.64 ms | 66.79 ms |
| d501a7e | 307.33 ms | 341.94 ms | 34.61 ms |
| 0ee65e9 | 321.06 ms | 361.24 ms | 40.18 ms |
| ed33deb | 334.19 ms | 362.30 ms | 28.11 ms |
| 9fbb112 | 401.87 ms | 515.87 ms | 114.00 ms |
| b8bd880 | 314.56 ms | 336.50 ms | 21.94 ms |
| 5b1a06b | 315.40 ms | 353.33 ms | 37.94 ms |
| 6edfca2 | 316.43 ms | 398.90 ms | 82.46 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 05aa61d | 0 B | 0 B | 0 B |
| bb0ff41 | 0 B | 0 B | 0 B |
| 806307f | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| d501a7e | 0 B | 0 B | 0 B |
| 0ee65e9 | 0 B | 0 B | 0 B |
| ed33deb | 1.58 MiB | 2.13 MiB | 559.52 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| b8bd880 | 1.58 MiB | 2.29 MiB | 722.92 KiB |
| 5b1a06b | 0 B | 0 B | 0 B |
| 6edfca2 | 1.58 MiB | 2.13 MiB | 559.07 KiB |
runningcode
left a comment
There was a problem hiding this comment.
looks good, i left a question for a discussion!
| } | ||
| } | ||
| } catch (e: InterruptedException) { | ||
| Thread.currentThread().interrupt() |
There was a problem hiding this comment.
do we need to surround this with try/catch as well?
| val token = encoderLock.tryAcquire(ENCODER_RELEASE_TIMEOUT_MS, MILLISECONDS) | ||
| if (token == null) { | ||
| options.logger.log( | ||
| WARNING, |
There was a problem hiding this comment.
ok so just to check my understanding, we try to get the lock only to make sure nobody else is holding it before calling release but if we can't acquire the lock, what state are we in? is this safe to skip the release? won't we cause a memory leak?
0xadam-brown
left a comment
There was a problem hiding this comment.
A few quick comments for your consideration (no blockers); otherwise lgtm
| encoderLock.acquire().use { | ||
| encoder?.release() | ||
| encoder = null | ||
| // close() is called inline from the lifecycle path (ReplayIntegration.stop/close), which holds |
There was a problem hiding this comment.
m: Thoughts about lifting this decision to the call site? That'd let us reduce how much we peg ReplayCache's implementation to its current caller.
Eg, we could:
- create a new ReplayCache.tryClose() method and have ReplayIntegration call it instead; or
- add a new ShutdownMode param to close() and have ReplayIntegration choose BEST_EFFORT.
Long-term side note: If we had a general pattern where, say, the Integration owned the execution context / threading decisions, we could leave everything beneath it thread _un_safe on the assumption that our Integrations would serialize execution via thread confinement by default. That'd make the bulk of our integration code much easier to implement and reason about.
I haven't looked in detail, but seems like that sort of pattern should be possible, given the very task-oriented nature of our processing + the fact that the host app needs very little from the SDK in real-time.
| wedge.countDown() | ||
| encoder.join(SECONDS.toMillis(10)) | ||
| } | ||
| } |
There was a problem hiding this comment.
l: Consider adding tests showing that the encoder is or isn't invoked depending on whether lock acquisition times out in ReplayCache.close().
📜 Description
Fixes an ANR where Android apps freeze on foreground/background transitions until the system kills the process.
A reporter pulled the full deobfuscated 131-thread dump from Play Console (the Sentry ANR event only carries
main, which is why the lock holder was invisible), giving us the exact chain:Two cooperating defects, so two bounds — both are needed:
SimpleVideoEncoder.drainCodec()had awhile (true)with no exit on theendOfStream = truepath. Some hardware encoders never emitBUFFER_FLAG_END_OF_STREAMaftersignalEndOfInputStream(), so the loop spun forever while holdingencoderLock. It now gives up after 10 consecutive no-progress iterations (~1s at the existing 100ms poll) and drops the remaining frames.ReplayCache.close()used an unboundedencoderLock.acquire(). It runs inline on the caller's thread fromBaseCaptureStrategy.stop(), whichReplayIntegration.stop()invokes while holdinglifecycleLock— that nesting is what promotes a stuck encoder into a frozen main thread. It now waits at most 2s and skips the release otherwise.The loop bound alone isn't sufficient: the native frames show
MediaCodec::dequeueOutputBuffer→AMessage::postAndAwaitResponse→ALooper::awaitResponsewithMediaCodec_looperidle.TIMEOUT_USECis handed to the codec looper, which is supposed to reply once it elapses — a dead looper never replies, andawaitResponsehas no deadline of its own. So a single call can never return, and only bounding the lock wait keeps the lifecycle path free.On the timeout path the (already-dead) codec is not released, leaking a native handle. That's the deliberate trade: leaking a handle in a process whose encoder is wedged beats freezing the app.
isClosed.set(true)still runs on every path, sincepersistSegmentValuesgates on it.The other four
encoderLock.acquire()sites inReplayCacheare unchanged — they all run on the replay worker, the thread that would be stuck, so a deadline there buys nothing.Adds
AutoClosableReentrantLock.tryAcquire(timeout, unit)(@ApiStatus.Internal, additive) for the second bound. Returns the token on success andnullon timeout, so callers must branch explicitly rather than silently no-op via?.use {}.Net effect: a wedged codec degrades to "replay stops working for this process" instead of "the app freezes."
💚 How did you test it?
ReplayShadowMediaCodecgained two hooks, since it ignorestimeoutUsand always eventually produces EOS:neverSignalEosandblockOnDequeue. New tests cover both bounds inReplayCacheTest, plus 4 fortryAcquireinAutoClosableReentrantLockTest.Both
ReplayCacheTestcases were mutation-checked — reverting each fix in turn makes the matching test fail with its assertion message rather than hang. Full:sentry-android-replay:testReleaseUnitTestpasses;apiDumpadds exactly the onetryAcquireline.No device repro is available (Redmi Note 14 Pro 5G / Pro+ 5G, Android 16, MediaTek), so the shadow-based tests are the verification of record.
📝 Checklist
sendDefaultPiiis enabled.🤖 Generated with Claude Code